-
Notifications
You must be signed in to change notification settings - Fork 2
feat: artifact support #122
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
77a15c8 to
2fb1bef
Compare
|
Hi there @bitwalker. Status update: This PR is almost ready for review, I'm doing some final polishing. Before merging this, I think it would be worthwhile to add an artifact field to at least one component, so that we can check that it works properly. So far, I've been testing this with a simple hello world binary. Maybe we could try adding the debugger, since that's where the release github action currently resides. Is the next branch ready for merging? Did you have something else in mind? |
| // bin/ directory which holds binaries. | ||
| let bin_dir = toolchain_dir.join("bin"); | ||
| if !bin_dir.exists() { | ||
| std::fs::create_dir_all(&bin_dir).with_context(|| { | ||
| format!("failed to create toolchain directory: '{}'", bin_dir.display()) | ||
| })?; | ||
| } | ||
|
|
||
| // lib/ directory which holds MASP libraries. | ||
| let lib_dir = toolchain_dir.join("lib"); | ||
| if !lib_dir.exists() { | ||
| std::fs::create_dir_all(&lib_dir).with_context(|| { | ||
| format!("failed to create toolchain directory: '{}'", lib_dir.display()) | ||
| })?; | ||
| } | ||
|
|
||
| // opt/ directory which holds symlinks to binaries in bin/. | ||
| // These are used in order to preserve a "midenup" compatible | ||
| // interface. This relies on the fact that clap uses argv[0] in order to | ||
| // display executable names names. These symlinks have the following format: | ||
| // `miden <component name>` | ||
| // Then, when `miden` is invoked, it uses these symlinks to execute the | ||
| // underlying binary. With this setup, `clap` displays the name as: `miden | ||
| // <component name>` instead of just `binary_name` when displaying help | ||
| // messages. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opt/, bin/ and lib/ did not change in this PR. I simply added these lines and comments to elaborate on their functionality.
f24bcb3 to
0a4ba74
Compare
d184473 to
c82f722
Compare
| {%- else if dep.path %}, path = "{{ dep.path }}" | ||
| {%- endif %} } | ||
| {%- endfor %} | ||
| curl = "{{ curl_version }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will use the same version of CURL as the midenup itself. This is obtained in the build.rs file.
manifest/channel-manifest.json
Outdated
| "artifacts": [ | ||
| { | ||
| "target": "zkvm-miden-unknown", | ||
| "uri": "https://github.com/lima-limon-inc/fabriz.io/releases/download/0.0/std.masp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These URLS are not intended to be merged and are only here to test this PR. They should be removed before merging.
30d37dd to
d0d2f32
Compare
|
Sidenote: I'll give the implementation a rework. I'll replace the triplet to url table with a list of urls instead to simplify things. |
|
@lima-limon-inc I finally got the the release workflow for v0.4.4 can be used with the v0.19 toolchain, upcoming releases will work with v0.20+. In the meantime, hopefully that provides all that is needed to validate a real component works with this feature. |
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
a747d42 to
68d1af9
Compare
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
68d1af9 to
f6eec13
Compare
081b138 to
501a0f9
Compare
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
501a0f9 to
4d295a2
Compare
@bitwalker awesome, thanks for the ping! I have just added the debugger in the 0.19.0 toolchain with its corresponding artifact field in this commit: a85fb79. It only required doing a small change to the way URI's were parsed, here: 4d295a2. Since the link did not contain the component name (i.e. "debugger"), but rather the repository name (i.e. "miden-debug"). However, this ended up simplifying the implementation all together and actually made the URI's more versatile, since now it can be an arbitrary string as long as the triplet is respected. I tested on my machine (Apple M3) and the debugger's binary was fetched successfully and I was able to execute it. Here's the output with the corresponding command: (Sidenote: the warning message stems from #123, which got fixed in #124). |
|
@lima-limon-inc Nice work! The next release of the compiler will have artifacts as well, just like One thing to note, This would mean that we'd end up with the following components currently:
The non-executable component names are a bit trickier, though I think currently there is only the standard library and protocol library that fall in this bucket. For those, I think we'll want to use the convention of naming the component after the package, e.g. Anyway, wanted to align on that since it came up. |
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
|
@bitwalker thanks ^_^.
Noted, I've pushed this commit 819ab9f which changes the component's name to
Great, thanks for the heads up! |
|
I'll do one last once-over of this today, and try to get it merged ASAP! |
Closes: #121
This PR adds artifact support, both for executables and libraries. With these changes,
midenupshould first try to download an artifact if it exists, if it doesn't or the download process fails, then it defaults to install the component from source.Implementation notes:
Artifactstruct holds a URI which is used to fetch the artifact, either from a URL (prefixed byhttps://) or from a file (prefixed byfile://). Each artifact only holds a specific triplet (for more information, see LLVM's triplet implementation), which is displayed in the URI itself, for example:https://github.com/0xMiden/miden-debug/releases/download/v0.4.4/miden-debug-aarch64-apple-darwin: : Holds themiden-debugger's binary foraarch64-apple-darwinhttps://github.com/0xMiden/miden-vm/releases/download/0.0/std.masp: Holds thePackagefor thestdlibrary. (Do note thatmasppackages are used by the MidenVM and therefore these URIs do not have a platform dependent triplet).src/external.rsfile holds functions that are imported into the install script by theinclude_strmacro. These were added in a separate file to enhance readability.